add list of list support to buttongroup widget #90

jo47011 · 2024-10-19T14:24:58Z

You can now add list of buttons to the BouttonGroup widget. These will be displayed in a grouped style. Added a simple example to ui_showcase.py:

# A simple ButtonGroup with nested lists of ToggleButtons for foobar
index_page.add_item(ButtonGroup("State of the foobar (grouped)", [
    [ToggleButton("Foo").connect(foo),ToggleButton("Bar", color='red').connect(bar)],
    [ToggleButton("Foobar", color='black').connect(foobar)],
]))

codecov · 2024-10-19T14:30:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.16%. Comparing base (905aed0) to head (f017ccd).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
+ Coverage   82.75%   83.16%   +0.41%     
==========================================
  Files          42       42              
  Lines        5608     5614       +6     
  Branches      865      761     -104     
==========================================
+ Hits         4641     4669      +28     
  Misses        743      743              
+ Partials      224      202      -22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jo47011 · 2024-10-19T17:07:14Z

example/pulseaudio_sink.py

@@ -35,10 +36,10 @@
 page.add_item(Slider("Volume").connect(volume.field('volume')))
 page.add_item(Slider("Balance").connect(volume.field('balance'), convert=True))
 page.add_item(Slider("Fade").connect(volume.field('fade'), convert=True))
-page.add_item(ButtonGroup("", [
+page.add_item(ButtonGroup("", cast(Iterable[AbstractButton], [


No idea why mypy wants this now here and not at other places, e.g. ui_showcase.py. Maybe you have an idea how to solve it without the cast. Looks kind of ugly.

MyPy tries to guess the type of that list here, based on the first common ancestor class of the two buttons it finds. Unfortunately it finds the common ancestor WebDisplayDatapoint[bool] instead of AbstractButton. So it complains that we are passing a list of ``WebDisplayDatapoint[bool]s instead of a list of AbstractButtons`. I assume that, without the new `Union` in the parameter type hint, MyPy was smart enough to find that `List[AbstractButton]`, as expected by the parameter, is also a valid type of that list, so it used that instead of trying to guess the type.

I think, the "correct" solution would be to explicitly type-hint the list:

buttons: List[AbstractButton] = [ ToggleButton(icon('volume mute')).connect(mute), DisplayButton(label=icon('power off')).connect(active), ] page.add_item(ButtonGroup("", buttons))

However, that also looks kind of ugly, especially in the example code. So maybe, we go for a simple type-ignore with a comment, like # type: ignore # MyPy has problems with guessing the correct type of the list of buttons.
Maybe, MyPy will learn this type inference eventually and we can remove the comment. (I will check that once in a while by running MyPy with --warn-unused-ignores locally.)

test/test_web.py

mhthies · 2024-10-20T08:43:55Z

The remaining MyPy issues are unrelated to this PR and should exist on the main branch, as well. I assume that they are caused by a MyPy update, including new typing semantics or checks. (The CI pipeline always uses the latest MyPy version.)

mhthies

Thank you very much for diving into this and contributing the patch! :)

I'm overall really happy with your patch (and that's saying something, as I'm known for my profound code reviews at work). ;)

Maybe you can add a unit test for the new usage of the ButtonGroup? It should create a ButtonGroup with multiple groups and check that all buttons are present. Maybe also check that the buttons are correctly grouped, but that's a bonus. I will not strictly expect the test for merging the PR, but it would be nice.

mhthies · 2024-10-20T08:29:05Z

shc/web/templates/widgets/buttongroup.htm

-<div class="ui buttons right floated">
-    {% for button in buttons %}
-        {{ render_button(button) }}
+<div class="ui spaced horizontal list right floated">


I like the use of ui horizontal list here. Sounds like the intended solution by SemanticUI. :)

mhthies · 2024-10-20T09:03:28Z

example/pulseaudio_sink.py

@@ -35,10 +36,10 @@
 page.add_item(Slider("Volume").connect(volume.field('volume')))
 page.add_item(Slider("Balance").connect(volume.field('balance'), convert=True))
 page.add_item(Slider("Fade").connect(volume.field('fade'), convert=True))
-page.add_item(ButtonGroup("", [
+page.add_item(ButtonGroup("", cast(Iterable[AbstractButton], [


MyPy tries to guess the type of that list here, based on the first common ancestor class of the two buttons it finds. Unfortunately it finds the common ancestor WebDisplayDatapoint[bool] instead of AbstractButton. So it complains that we are passing a list of ``WebDisplayDatapoint[bool]s instead of a list of AbstractButtons`. I assume that, without the new `Union` in the parameter type hint, MyPy was smart enough to find that `List[AbstractButton]`, as expected by the parameter, is also a valid type of that list, so it used that instead of trying to guess the type.

I think, the "correct" solution would be to explicitly type-hint the list:

buttons: List[AbstractButton] = [ ToggleButton(icon('volume mute')).connect(mute), DisplayButton(label=icon('power off')).connect(active), ] page.add_item(ButtonGroup("", buttons))

However, that also looks kind of ugly, especially in the example code. So maybe, we go for a simple type-ignore with a comment, like # type: ignore # MyPy has problems with guessing the correct type of the list of buttons.
Maybe, MyPy will learn this type inference eventually and we can remove the comment. (I will check that once in a while by running MyPy with --warn-unused-ignores locally.)

test/test_web.py

mhthies · 2024-10-20T09:17:41Z

shc/web/widgets.py

@@ -322,19 +334,28 @@ class ButtonGroup(WebPageItem):
    `Connectable`.

    :param label: The label to be shown left of the buttons
-    :param buttons: List of button descriptors
+    :param buttons: List or a List if List of button descriptors


One afterthought: Can you please add a more detailed description on how to use the parameter, i.e. what effect the single list of button descriptors and the list of lists of button descriptors will have? You can either write it in the parameter description or as a separate paragraph above.

jo47011 · 2024-10-20T19:31:33Z

Ready to go. Let me know whether you have any additional change requests. No problem, in my company they also "fear" my pedantic reviews 😄

jo47011 · 2024-10-20T19:38:53Z

As for being pedantic: I can see that your code is not consistently formatted. Running black over the repo gives me 68 files reformatted. Mainly replacing single ticks with double ticks which are used interchangeably. But also some general formatting. If you want I can open a new PR once we have merged this one just w/ those changes. What do you think?

mhthies · 2024-10-26T10:50:57Z

As for being pedantic: I can see that your code is not consistently formatted. […] If you want I can open a new PR once we have merged this one just w/ those changes. What do you think?

I recently thought about replacing flake8 with ruff, which would also include the black functionality. So, I'm open to do this, but would suggest migrating to ruff directly.

We should also update the README file section "Tests and Code Style", when this is changed, and the CI script to check formatting with black/ruff.

Mainly replacing single ticks with double ticks which are used interchangeably.

I actually used both on purpose with some kind of system (single quotes for key-like string literals, double quotes for other strings), but I have to admit that this is not really consistent, there's lot of grey area and nobody else seems to use this convention. So I'm open to change this. :)

jo47011 · 2024-10-28T07:13:56Z

Thanks for your approval. But I have no rights to merge the PR:

I just changed to ruff for my shc implementation. So that's a good match. I will put it on the list fort the shc repo. Will be a few days down the road 😉

add list of list support to buttongroup widget

d56cfce

jo47011 added 3 commits October 19, 2024 16:36

remove package-lock.json

cf94d0e

fix: flake issues

6a93233

fix: tests

97fd8b1

jo47011 commented Oct 19, 2024

View reviewed changes

test/test_web.py Show resolved Hide resolved

mhthies assigned mhthies and unassigned mhthies Oct 20, 2024

mhthies self-requested a review October 20, 2024 08:27

mhthies reviewed Oct 20, 2024

View reviewed changes

jo47011 added 2 commits October 20, 2024 21:25

add: parameter description details

920f207

add: button group test

531087b

jo47011 requested a review from mhthies October 20, 2024 19:31

mhthies approved these changes Oct 26, 2024

View reviewed changes

fix: flake issue

f017ccd

mhthies merged commit ec6f86e into mhthies:main Oct 30, 2024
9 checks passed

jo47011 deleted the add_nested_groups_to_buttongroup branch November 30, 2024 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add list of list support to buttongroup widget #90

add list of list support to buttongroup widget #90

jo47011 commented Oct 19, 2024 •

edited

Loading

codecov bot commented Oct 19, 2024 •

edited

Loading

jo47011 Oct 19, 2024

mhthies Oct 20, 2024

mhthies commented Oct 20, 2024

mhthies left a comment

mhthies Oct 20, 2024

mhthies Oct 20, 2024

mhthies Oct 20, 2024

jo47011 commented Oct 20, 2024

jo47011 commented Oct 20, 2024

mhthies commented Oct 26, 2024 •

edited

Loading

jo47011 commented Oct 28, 2024

add list of list support to buttongroup widget #90

add list of list support to buttongroup widget #90

Conversation

jo47011 commented Oct 19, 2024 • edited Loading

codecov bot commented Oct 19, 2024 • edited Loading

Codecov Report

jo47011 Oct 19, 2024

Choose a reason for hiding this comment

mhthies Oct 20, 2024

Choose a reason for hiding this comment

mhthies commented Oct 20, 2024

mhthies left a comment

Choose a reason for hiding this comment

mhthies Oct 20, 2024

Choose a reason for hiding this comment

mhthies Oct 20, 2024

Choose a reason for hiding this comment

mhthies Oct 20, 2024

Choose a reason for hiding this comment

jo47011 commented Oct 20, 2024

jo47011 commented Oct 20, 2024

mhthies commented Oct 26, 2024 • edited Loading

jo47011 commented Oct 28, 2024

jo47011 commented Oct 19, 2024 •

edited

Loading

codecov bot commented Oct 19, 2024 •

edited

Loading

mhthies commented Oct 26, 2024 •

edited

Loading